Repository navigation
os/v3/puppy: a console-TUI dialog loses its question before its hint - #1028
Conversation
|
#1028 ready for review at The one measurement to check: a pty whose pane is exactly 40x11, key 1. Before this branch that screen drew the key-confirm question with no hint line at all ( Independent of #1026 ( |
1c50d98 to
463ad87
Compare
`draw()` sizes the modal box to its body and clamps it to the frame (`bh = (body.len() + 2).min(area.height)`), then
draws the body with `Paragraph`, so a body that does not fit is clipped at its END - and the last line of every dialog
is the hint, the only row that says how to answer.
Measured on the shipped static binary before this commit (main `434db3d2`, `0edbe573…`), real ptys, `TERM=linux`, key
**1** on a blank-key box:
80x25, 60x12 `y mints one and restarts Lemonade, n cancels` intact
40x12 `y mints one and restarts` half
40x11, 40x10, 30x12, 24x12 absent
`Modal::Confirm { yes: Yes::Reboot }` carries `y reboots the box, n cancels` and `Modal::Power` carries
`r reboots, p powers off, any other key cancels`, so in a 40x11 ssh window a screen that is about to reboot the box
explains neither key that reboots it - while `y` still does what the question says. No gate impact (the gates run at
80x25 and 240x67, where the whole box fits).
The fix is the same rule the rest of this file follows - the block's most load-bearing row is the last one to go:
the dialog is now `title | question | hint` as three sections, and a frame too short for all of it loses the question
rows first (whole rows at a time), then the title, and never the hint.
40x12 after key 1: hint whole (`y mints one and restarts` / `Lemonade, n cancels`), one question row given up
40x11 : hint whole, two question rows given up
24x12 : hint whole and wrapped (`y mints one and` / `restarts Lemonade, n` / `cancels`)
80x25, 240x67 : unchanged - the whole box, byte-identical frames
`the_modal_hint_is_never_the_row_that_goes` renders four dialogs (reboot confirm, blank-key confirm, power, the API
key info box) at 24x12, 30x12, 40x10, 40x11, 40x12, 60x12, 80x25 and 240x67, and requires the frame to be the terminal
and the hint to be on screen, whole, at every one of them. Putting the old body construction back turns it red at
24x12; the fix turns it green.
Measured on the static binary `42e11547b870428c` (1,556,736 B, no `PT_INTERP`): `cargo test` 47/47, clippy
`--all-targets -D warnings` clean, `cargo fmt --check` clean, `tools/copyright.py --check` rc 0,
`--snapshot 80x25 --fixture` matches the committed frame. 2 files +~85/-17, no new files, no gate file touched, no
image build, no board run, no device ticket.
Closes #1027
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
463ad87 to
7ba4e7f
Compare
|
Head moved to Artifact hash correction, so nobody chases a ghost: the tree of this head builds CI is re-running on the new head. |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 463ad8792
$ cd os/v3/puppy/console-tui && cargo test --offline
test result: ok. 47 passed; 0 failed; 0 ignored
The added test is the_modal_hint_is_never_the_row_that_goes, and it pins the property the diff implements: for four real dialogs (reboot confirm, blank-key confirm, power menu, info box) at 24x12 / 30x12 / 40x10 / 40x11 / 40x12 / 60x12 …, the hint row is always drawn, so y reboots the box, n cancels can no longer be the thing that falls off a frame where y still reboots.
The code change is the right shape for it: title_rows / text_rows / hint_rows are wrapped separately, and the trimming loop takes the explanation first (whole rows), then the title, never the hint; the box height is recomputed from what survived, so bh can no longer exceed the frame and ratatui cannot clip the end.
Note for the merge order, not a defect: this and #1026 both rewrite regions of os/v3/puppy/console-tui/src/ui.rs and its README (this one the modal block at ~851, #1026 the footer at ~704 plus the draw() prologue). They are independent hunks and both are green, but whichever lands second needs its rebase re-run — the §"frame is the terminal" tests are the ones that would catch a bad merge.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 7ba4e7f33
Rebase only, checked as content rather than as counters: the +/- lines of the PR's own diff are identical at 463ad8792 (the head I reviewed) and at 7ba4e7f (the diff against the moving merge base showed +111 and now +109 only because main's context moved). cargo test --offline was 47 passed / 0 failed at 463ad8792, and ui.rs is unchanged since, so that result carries.
Same merge-order note as before: this and #1026 both rewrite console-tui/src/ui.rs (modal block here, footer + draw() prologue there); whichever lands second needs its rebase re-run, and the frame-is-the-terminal tests are what would catch a bad merge.
|
Updated to main ( |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 12907048
Rebase-only, verified twice over:
own contribution (+/- lines vs merge base), 7ba4e7f33 vs 12907048:
+109/-8, sha256 of the diff lines = 1e496b48eadb (both heads)
$ cd os/v3/puppy/console-tui && cargo test --offline (at 12907048, on main 8c1eaea3)
test result: ok. 47 passed; 0 failed; 0 ignored
12907048 is a merge of main (Merge branch 'main' into os/v3-tui-modal), so the diff against the merge base is identical to the head I reviewed; I re-ran the suite at the new head anyway since this and #1026 both rewrite ui.rs. 47/47, including the_modal_hint_is_never_the_row_that_goes (4 dialogs × 6 sizes), which is the property that closes #1027.
Review stands as posted. The merge-order note is the only thing left: this and #1026 are independent hunks of the same file, so whichever lands second should re-run cargo test after its merge — the frame-is-the-terminal assertions are what would catch a bad merge, and they are cheap.
…e order is conflict-free No behaviour change, no line of logic moved: the modal test now sits above `the_menu_rule_gives_way_before_an_action_does` (which #1026 appends after) and the README paragraph now sits above the MENU-rule paragraph, so the two PRs no longer share merge context. Measured: before this, merging #1026 into this branch conflicted on both os/v3/puppy/console-tui/{README.md,src/ui.rs}; after it, both orders merge cleanly and `cargo test` is 48/48 on the combined tree.
|
Head Measured first: test-merging #1026 ( One bookkeeping nit, recorded rather than hidden: both PRs set the README's test count to 47, so after both land the line says 47 while the suite is 48. Whichever merges second can bump it, or tell me and I will - it is a one-word README change and I did not want to put a third push on a PR that is already READY for a count. The reviewed diff of the code is unchanged: |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at deb9a06
Merge-only rebase, and this push also settles the merge-order question I raised, so I checked it the way it should be checked — by merging #1026 into this branch and running the suite:
own contribution (+/- lines vs merge base), 12907048 vs deb9a06: +109/-8, hash 1e496b48eadb (identical)
git merge origin/main (main = 9315118, i.e. #1026 already in)
-> clean, no conflicts ("placement only: keep this PR's hunks clear of #1026's, so either merge order is conflict-free")
cargo test --offline
-> test result: ok. 48 passed; 0 failed; 0 ignored
48 = 46 base + a_message_gives_way_before_an_action_does (#1026, now on main) + the_modal_hint_is_never_the_row_that_goes (this PR). So both console-TUI PRs' tests run together against the merged tree and both pass — either merge order is safe, and the "whichever lands second must re-run" note is discharged.
Review stands as posted.
|
#1026 merged as Two consequences worth stating because my earlier comments quote the old numbers:
Everything else stands: the reviewed modal diff is unchanged ( (A previous version of this comment was mangled by shell backticks in transit; this one is the correction.) |
|
Re-verdict requested at For a fast re-check, the two things that could have moved: |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 38b8e1d8
Both pushes checked with the instrument that can actually see a reordering: the PR's own +/- lines.
deb9a06 (placement only) vs 12907048 (the head I reviewed):
own-diff hash 1e496b48eadb, identical -- +109/-8 both
git diff 12907048 deb9a06: 98 lines moved in ui.rs + the README paragraph, i.e. main's MENU-rule test and
paragraph relocated around this PR's hunk. Because the moved text exists identically at both ends, a move
cancels out of the diff, which is exactly why the hash did not move: nothing this PR adds changed.
38b8e1d8 (merge main = #1026 aboard) vs 12907048:
the ONLY difference in the PR's own lines is the README suite count, 47 -> 48:
-cargo test # 47 tests: ...
+cargo test # 48 tests: ...
so the modal branch and `the_modal_hint_is_never_the_row_that_goes` are byte-identical to what I reviewed.
$ cd os/v3/puppy/console-tui && cargo test --offline (at 38b8e1d8, main 9315118 = #1026 merged)
test result: ok. 48 passed; 0 failed; 0 ignored
48 = 46 base + #1026's a_message_gives_way_before_an_action_does + this PR's the_modal_hint_is_never_the_row_that_goes, and the count in the README now matches what the suite actually prints. I had already merge-tested this pair at deb9a06 (clean merge, 48/48); with #1026 now in main, this head is that same tree and the same result, so the merge-order question is settled in both directions.
Review stands as posted — the hint is never the row that goes, the trimming loop takes the explanation first and then the title, and bh is recomputed from what survived so ratatui cannot clip the end.
Not re-derived: the "80x25/240x67 frames byte-identical" claim. The committed test asserts the frame-is-the-terminal invariant and the gate strings at those sizes and passes, which covers the property from inside the suite; a frame-by-frame comparison against the base would be the only way to check byte-identity and the sizes you sweep are already covered by the 4 dialogs × 6 sizes assertions.
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
os/v3/puppy: a console-TUI dialog loses its question before its hint
draw()sizes the modal box to its body and clamps it to the frame (bh = (body.len() + 2).min(area.height)), thendraws the body with
Paragraph, so a body that does not fit is clipped at its END - and the last line of every dialogis the hint, the only row that says how to answer.
Measured on the shipped static binary before this commit (main
434db3d2,0edbe573…), real ptys,TERM=linux, key1 on a blank-key box:
80x25, 60x12
y mints one and restarts Lemonade, n cancelsintact40x12
y mints one and restartshalf40x11, 40x10, 30x12, 24x12 absent
Modal::Confirm { yes: Yes::Reboot }carriesy reboots the box, n cancelsandModal::Powercarriesr reboots, p powers off, any other key cancels, so in a 40x11 ssh window a screen that is about to reboot the boxexplains neither key that reboots it - while
ystill does what the question says. No gate impact (the gates run at80x25 and 240x67, where the whole box fits).
The fix is the same rule the rest of this file follows - the block's most load-bearing row is the last one to go:
the dialog is now
title | question | hintas three sections, and a frame too short for all of it loses the questionrows first (whole rows at a time), then the title, and never the hint.
40x12 after key 1: hint whole (
y mints one and restarts/Lemonade, n cancels), one question row given up40x11 : hint whole, two question rows given up
24x12 : hint whole and wrapped (
y mints one and/restarts Lemonade, n/cancels)80x25, 240x67 : unchanged - the whole box, byte-identical frames
the_modal_hint_is_never_the_row_that_goesrenders four dialogs (reboot confirm, blank-key confirm, power, the APIkey info box) at 24x12, 30x12, 40x10, 40x11, 40x12, 60x12, 80x25 and 240x67, and requires the frame to be the terminal
and the hint to be on screen, whole, at every one of them. Putting the old body construction back turns it red at
24x12; the fix turns it green.
Measured on the static binary
c046551247f07569(1,556,728 B, noPT_INTERP):cargo test47/47, clippy--all-targets -D warningsclean,cargo fmt --checkclean,tools/copyright.py --checkrc 0,--snapshot 80x25 --fixturematches the committed frame. 2 files +111/−8, no new files, no gate file touched(
vm-check.sh,puppy-check.sh,menugate.shuntouched), no image build, no board run, no device ticket.Evidence
434db3d2,0edbe573…) in real ptys whose pane is exactly the sizegiven: key 1 at 40x12 ->
y mints one and restarts; 40x11 / 40x10 / 30x12 / 24x12 -> no hint line at all.y mints one and restarts/Lemonade, n cancels),24x12 -> the hint whole and wrapped over three rows.
the_modal_hint_is_never_the_row_that_goes(4 dialogs × 8 sizes) is red with the old body construction at 24x12 andgreen with the fix.
renders_at_the_two_console_sizes.Not in this PR